Repository navigation
Conversation
Member
Author
|
@claude review |
|
LinuxJedi
force-pushed
the
fenrir-wolfcrypt-batch
branch
from
November 9, 2026 17:21
f997be3 to
1e26eb7
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
LinuxJedi
force-pushed
the
fenrir-wolfcrypt-batch
branch
from
November 9, 2026 17:36
1e26eb7 to
04a6f4a
Compare
wolfSSL-Fenrir-bot
previously requested changes
Nov 9, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12067
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-bugs
Not reviewed (no scan target covers them): tests/api/test_asn.h, tests/api/test_wolfmath.h
Coverage: 8 of 8 in-scope changed file(s) opened by the reviewer
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Max
LinuxJedi
force-pushed
the
fenrir-wolfcrypt-batch
branch
3 times, most recently
from
November 10, 2026 11:48
b3bbf42 to
ea5586d
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #12067
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-bugs
Not reviewed (no scan target covers them): tests/api/test_asn.h, tests/api/test_wolfmath.h
Coverage: 7 of 7 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
wolfSSL-Fenrir-bot
dismissed
their stale review
November 10, 2026 13:13
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
The template DecodeSEP() assigned cert->hwType and cert->hwSerialNum without checking whether they were already set. A subjectAltName with several HardwareModuleName otherNames overwrote the buffers of the previous one on each entry, and FreeDecodedCert() freed only the last pair, leaking memory on every parse of such a certificate. Reject a second HardwareModuleName with ASN_PARSE_E, as the original parser already does. Writing a test for this showed that the template never decoded the HardwareModuleName contents. The OtherName template takes the value's SEQUENCE as a whole, so its hwType and hwSerialNum child items were never filled in. Both were stored as zero-length buffers, and the parse failed with MEMORY_E wherever a zero-byte allocation returns NULL. Decode the SEQUENCE contents in DecodeSEP() with a template of their own, once the type-id is known to be a HardwareModuleName, and set oidIgnoreType on hwType before parsing. Other otherName types that have a SEQUENCE value are still accepted as before. Add test_DecodeAltNames_dup_hw_module_name. It checks that a single HardwareModuleName is decoded, that a second one is rejected, and that a permanentIdentifier otherName is still accepted.
wc_InitMutex() for the MAXQ10XX shared hardware mutex returned on failure without closing the shared memory descriptor or unmapping the mapping, and it set wcCryptHwSharedMutexPtr before the mutex was initialized. When it created the shared memory object and then failed, the object was left in place without an initialized mutex. The next init attempt opened the existing object, skipped initialization because it had not created it, and returned success, so callers locked a mutex that was never initialized. Close the descriptor on every path, unmap on failure, and destroy the mutex attributes. Remove a newly created object whose mutex was not initialized. Publish wcCryptHwSharedMutexPtr only once initialization has succeeded. Also free the per-thread lock count in maxq_LockMutex() when pthread_setspecific() fails, instead of leaking it.
Outside FIPS builds, sp_clear() zeroed only the digits below used, with plain stores. Data is not zeroed when used decreases, so digits above used could still hold part of an earlier, larger value, and the stores could be removed as dead when the sp_int was not used again. Callers use mp_clear() on secret values, so these could be left in memory. Make sp_clear() wipe all a->size digits with ForceZero(), as sp_forcezero() and FIPS builds already did, and have sp_forcezero() call it. sp_clear() now relies on size matching the storage. The template CRL Number decode used GetInt(), which initializes the mp_int it decodes into a second time, setting size to the maximum. With HAVE_WOLF_BIGINT, the mp_clear() done when the bigint allocation fails then overran the CRL Number's storage. Decode into the initialized mp_int instead. Add test_mp_clear_zeroize, which checks that mp_clear() leaves no non-zero digits after the value has been made smaller.
Outside FIPS builds, the heap math mp_clear() zeroed only the digits below used, with plain stores, and then freed the digit buffer. Any digits from used up to alloc were freed without being wiped, and the stores could be removed as dead because the buffer is freed straight after. mp_forcezero() also wiped only the used digits. Make mp_forcezero() wipe all alloc digits with ForceZero(), and have mp_clear() call it in all builds, as FIPS builds already did. Keep the HAVE_WOLF_BIGINT handling from mp_clear(), which frees the raw buffer even when dp is NULL. Extend test_mp_clear_zeroize to heap math. It uses allocator callbacks to check that the freed digit buffer has been wiped in full.
Outside FIPS builds, fp_clear() wiped the digits with a plain memset. fp_clear() is normally the last use of the fp_int, so the compiler may remove the memset as a dead store. For example, with clang -O2 the Miller-Rabin code is inlined into mp_prime_is_prime() and mp_prime_is_prime_ex(). After the last round, all three fp_clear() wipes in mp_prime_is_prime() and four of the five in mp_prime_is_prime_ex() are removed. RSA key generation uses these to test candidate primes, so values derived from the secret primes, such as p - 1, were left on the stack. Make fp_clear() call fp_forcezero() in all builds, as FIPS builds already did. It wipes the same digits with ForceZero(), whose barrier keeps the stores.
wc_InitMutex() for the MAXQ10xx hardware mutex created the shared memory object and initialized the process-shared mutex in it as separate steps, and decided whether to initialize it from which of two shm_open() calls succeeded. A process that opened the object after another one created it, but before that one initialized the mutex, skipped initialization and could take a lock that the creator's pthread_mutex_init() then reset, leaving both processes holding the mutex. Two processes that both found no object also raced on O_EXCL, and the loser failed outright. Open the object with O_CREAT and hold a record lock on it while checking and initializing it, with a process-local mutex for the threads of a process, which record locks do not exclude. The system releases the record lock if its holder exits, so a process killed during initialization does not block the others. The size of the object now records whether the mutex is ready: it is one byte larger than the mutex while being initialized, and the size of the mutex once ready. An object that is empty, or that a process left part way through initializing, is initialized again, so a failed initialization no longer needs to remove the object. An existing object is no longer truncated, and one of any other size is rejected. A ready object has the same layout and size as before, so one left in place by an earlier build is still used. Make the mutex robust, so that a process that exits while holding it, part way through using the MAXQ10xx, no longer blocks every other process: the next process to lock it gets EOWNERDEAD, marks it consistent and carries on. A mutex initialized by an earlier build is not robust, and works as before until its object is removed. Also give maxq_CryptHwMutexTryLock() a prototype, which clang warns about without.
mp_grow() enlarged the digit array with XREALLOC(). When realloc() cannot grow a block in place, it copies it to a new block and frees the original without wiping it, leaving a copy of the value in freed memory. mp_grow() grows mp_ints that hold private key values and intermediates, and with MP_PREC set to 1 it reallocates often. Allocate the new array, copy the digits across, then wipe and free the old one. Add a test, with a realloc callback that always moves the block, that checks mp_grow() wipes the array it replaces and keeps the value.
LinuxJedi
force-pushed
the
fenrir-wolfcrypt-batch
branch
from
November 11, 2026 18:46
ea5586d to
a86f6b8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes for seven wolfCrypt issues, one commit each. Based on master, not stacked on another PR.
Bignum zeroization
sp_clear()(sp-math-all). Outside FIPS builds it zeroed only the digits belowused, with plain stores. Digits aboveusedcan still hold part of an earlier, larger value, and the stores can be removed as dead. It now wipes allsizedigits withForceZero(), assp_forcezero()and FIPS builds already did. This relies onsizematching the storage. The template CRL Number decode initialized a key-sizedsp_inta second time throughGetInt(), which callsmp_init()and so setssizeto the maximum. WithHAVE_WOLF_BIGINT, themp_clear()done when the bigint allocation fails then overran the CRL Number's storage. It now decodes into the initializedmp_intwithGetASN_MP_Inited, asDecodeECC_DSA_Sig_Ex()already does forrands.Every other
INIT_MP_INT_SIZE()andmp_init_size()site, and the functions thosemp_ints are passed to, was checked for the same pattern. The one other case,wc_ecc_verify_hash()inNO_ASNbuilds, has since been fixed on master.mp_clear()(heap math). Freed the digit buffer after wiping only the digits belowused, also with plain stores.mp_forcezero()now wipes allallocdigits withForceZero(), andmp_clear()calls it in all builds, as FIPS builds already did.mp_grow()(heap math). Grew the digit array withXREALLOC(), so each timerealloc()moved the block, the old copy of the value was freed without being wiped. WithMP_PRECset to 1 this happens often, including for private key values. It now allocates the new array, copies the digits across, and wipes and frees the old one.fp_clear()(fastmath). Used a plainmemset(), which the compiler can drop becausefp_clear()is usually the last use of thefp_int. With clang -O2, after the last Miller-Rabin round all three wipes inmp_prime_is_prime()and four of the five inmp_prime_is_prime_ex()are removed, leaving values derived from RSA candidate primes on the stack. It now callsfp_forcezero()in all builds.MAXQ10XX shared hardware mutex
wc_InitMutex()leaked the shared memory descriptor and mapping on failure, and setwcCryptHwSharedMutexPtrbefore the mutex was initialized. A failure after creating the object left it in place without an initialized mutex, and the next init then used it as is.maxq_LockMutex()also leaked the per-thread lock count whenpthread_setspecific()failed.pthread_mutex_init()could then reset a lock the other process held, leaving both holding the mutex. Two processes that both found no object also raced onO_EXCL, and the loser failed. The object is now opened withO_CREAT, and a record lock on it (fcntl(F_SETLKW)) is held while it is checked and initialized. The system releases that lock if its holder exits, so a process killed during initialization does not block the others. A process-local mutex covers the threads of a process, which record locks do not exclude. The object's size records whether the mutex is ready: one byte larger than the mutex during initialization, the size of the mutex once ready. An empty object, or one left part way through initialization, is initialized again. A ready object has the same layout and size as before, so one left in/dev/shmby an earlier build is still used.EOWNERDEAD, marks it consistent and carries on. A mutex initialized by an earlier build is not robust, and works as before until its object is removed. This is in the same commit as the race fix, which also givesmaxq_CryptHwMutexTryLock()a prototype, as clang warns without one.ASN.1 template parser
DecodeSEP()did not reject a second HardwareModuleName, so each one overwrote and leaked the previoushwTypeandhwSerialNum. It also never decoded the HardwareModuleName contents: both fields came out zero-length, and the parse failed withMEMORY_Ewherever a zero-byte allocation returns NULL. The contents are now decoded with a template of their own, and a second HardwareModuleName is rejected withASN_PARSE_E, as the original parser already does. Other otherName types with a SEQUENCE value, such as permanentIdentifier, are still accepted.Cost of the wider wipes, in callgrind instruction counts on arm64 Linux (gcc -O2, ECDH + ECDSA sign/verify on P-256, RSA-2048 and FFDHE-2048): sp-math-all ECC +1.5%, fastmath and heap math ECC about +0.5%, RSA and DH within 0.3%. The
mp_grow()change adds nothing: heap math runs 0.6% to 2% fewer instructions than withXREALLOC().Testing
test_DecodeAltNames_dup_hw_module_name,test_mp_clear_zeroize(sp-math-all, fastmath and heap math) andtest_mp_grow_zeroize(heap math).make checkon the default build and--enable-all.unit.teston--enable-heapmathand--enable-fastmath.testwolfcrypton the default build,--enable-smallstack,--enable-alland--enable-all --enable-smallstack, plusunit.teston both--enable-allbuilds.testwolfcryptandunit.teston--enable-heapmath, with and without--enable-smallstack. The CRL Number overflow was reproduced with ASan on--enable-crl --enable-pkcs11(which definesHAVE_WOLF_BIGINT) by failing each allocation in turn inwolfSSL_CertManagerLoadCRLBuffer(). Before the change, failing the bigint allocation overflows the stack insp_clear(). After it, no failure point reports an error.--enable-sep --enable-asn=original, with no leaks reported byWOLFSSL_TRACK_MEMORY.--enable-cryptonlywith-DMAXQ10XX_MUTEX -DWOLFSSL_CRYPT_HW_MUTEX=1). It was tested on arm64 Linux with a harness that injects failures into the pthread calls with-Wl,--wrapand runs several processes. Before these changes, it reproduces the leaked lock count, the leftover object being used without initialization, the create/init race (both processes holding the mutex), and an empty or undersized object being used as is. It also checks that a process killed while initializing the mutex does not block the next process, which initializes it again, and that a process killed while holding the mutex does not block the next one, whether that one locks or trylocks it. Before these changes, the next process hangs in both cases. If the mutex cannot be made consistent, locking it fails rather than blocks. All pass with this branch. 16 processes making 2000 locked increments each of a shared counter reach the exact total, and when 8 of them are killed part way, the other 8 still finish. The mutex code builds with-Wall -Wextra -Werrorunder gcc and clang.Checklist